Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow specifying workflow id when starting the workflow #1942

Closed
wants to merge 1 commit into from
Closed

Allow specifying workflow id when starting the workflow #1942

wants to merge 1 commit into from

Conversation

gnagy
Copy link

@gnagy gnagy commented Nov 26, 2023 •
edited
Loading

What was changed

For now this is an experiment to gather feedback.

  • Add a new method to WorkflowStub: WorkflowExecution startWithId(String workflowId, Object... args);
  • Add a new annotation @WorkflowId for a paramater on a @WorkflowMethod
  • Extend WorkflowInvocationHandler: if a @WorkflowId is specified, start workflow with startWithId()
  • Added tests

Why?

Currently the workflowId can only be set in WorkflowOptions, along with other options that are basically static config for a workflow instance, whereasworkflowId differs typically between workflow executions.

This makes injecting pre-configured workflow instances where they are to be started virtually impossible, which adds noise to the call site. With this new API, such code becomes possible (spring-like pseudo-code). The idea is that ExampleWorkflowImpl could be created by an IOC container for each request (e.g. in spring Request scope).

@Controller
class WorkflowController {

  @Autowired private ExampleWorkflow workflow;

  @PostMapping
  public void start(UUID idempotencyKey, Data data) {
    workflow.execute(idempotencyKey, data)
  }
}


@WorkflowInterface
public interface ExampleWorkflow {
  @WorkflowMethod
  String execute(@WorkflowId UUID idempotencyKey, Data data);
}

Checklist

  1. Closes: N/A

  2. How was this tested: With the added junit tests

  3. Any docs updates needed? Probably, as this is a public API addition.

@gnagy gnagy requested a review from a team as a code owner November 26, 2023 18:15
@CLAassistant
Copy link

CLAassistant commented Nov 26, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Quinn-With-Two-Ns
Copy link
Contributor

WorkflowStub is meant to represent a signal workflow execution. I am not a fan of adding multiple ways to specify workflowID overwriting each other. If you do want this functionality in your own code it seems you could easily provide this in wrapper class or utility function and does not need to be part of the SDK.

I am strongly against adding a WorkflowId to WorkflowMethod since I don't see a strong value in forcing a certain workflow type to always have the same ID. It also wouldn't work if the workflow is launched through an untyped stub, I could see that causing a lot of confusion for users.

@Quinn-With-Two-Ns
Copy link
Contributor

My apologize, didn't mean to immediately close

@gnagy
Copy link
Author

gnagy commented Nov 26, 2023

Thank you for the feedback.

I am strongly against adding a WorkflowId to WorkflowMethod since I don't see a strong value in forcing a certain workflow type to always have the same ID.

To clarify my idea, @WorkflowId is optional, meaning users don't have to add it to their @WorkflowMethod signature. However, if specified, the ID will be taken from the method argument, so ID is not the same for a certain workflow type -- or maybe I'm misunderstanding your comment?

I am not a fan of adding multiple ways to specify workflowID overwriting each other

I see your point. I was wondering about it too, and ended up implementing it in a way where if @WorkflowId parameter is null, the ID is taken from WorkflowOptions thus facilitating a fallback to default mechanism. So, if a caller doesn't specify a workflow id (e.g. in a http request) then it can fall back to some pre-defined value. This could be made nicer with Optional of course.

This is totally opt-in, if there is no parameter annotated with @WorkflowId, the behavior is exactly as before.

It also wouldn't work if the workflow is launched through an untyped stub, I could see that causing a lot of confusion for users.

For untyped stubs the original start() method is left unaffected. On one hand not breaking APIs is a good thing, also it just seemed easier to implement with a separate startWithId() method.

@Quinn-With-Two-Ns
Copy link
Contributor

Quinn-With-Two-Ns commented Nov 26, 2023 •
edited
Loading

My issue with adding WorkflowId to WorkflowMethod is it is not a common Temporal pattern, provides multi ways to specify workflow ID and would not be evenly applied to all invocations of a workflow causing confusion.

@Quinn-With-Two-Ns
Copy link
Contributor

I understand you left start() unchanged, that does not mean the new API is something Temporal would want to add for the reasons I listed above.

@gnagy
Copy link
Author

gnagy commented Nov 26, 2023

Ok thanks. I'll explore solving my needs with a wrapper class / utility function instead.

@gnagy gnagy closed this Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gnagy @CLAassistant @Quinn-With-Two-Ns